Parse command line arguments as a std::string #330

huangzonghao · 2022-05-01T04:05:00Z

A straightforward implementation for #329. Works fine for enabling local program to be called from browser via system protocols. Known limitation includes no bash variable and wildcard expansion. Haven't tested for corner cases.

This change is

This is the simplest implementation of the functionality described in the short description. This patch only deals with the arguments seperated by whitespaces, and process them as is, without any shell processing like expanding wildcards or replacing variables.

eyalroz · 2022-05-01T11:35:26Z

The API here should probably not be an std::string, but rather a string view. Unfortunately, this is not available before C++17, so - a const *char and length, i.e. const char* argv, std::size_t argv_length; or just const char* argv if it's a '\0'-terminated string. Once that is in, you could add a convenience method taking a const std::string&. I would avoid the unnecessary copying.

jarro2783 · 2022-05-02T02:18:58Z

src/example.cpp

@@ -185,5 +198,8 @@ int main(int argc, const char* argv[])
 {
  parse(argc, argv);

+  // Parse argv as string
+  parse(argc, argv, true);


I don't think adding all of this to the example is necessary. I would rather just have a test case that calls the new function.

jarro2783 · 2022-05-02T02:19:24Z

include/cxxopts.hpp

@@ -1772,6 +1772,9 @@ namespace cxxopts
    ParseResult
    parse(int argc, const char* const* argv);

+    ParseResult
+    parse(std::string argv_str);


I think this should take a constref.

jarro2783 · 2022-05-02T02:23:14Z

include/cxxopts.hpp

+    tokens.push_back(token);
+  }
+
+  const char** argv = new const char*[tokens.size()];


Can you use a smart pointer here?

…mpilation warnings of 95c7709

eyalroz

There is too much redundant copying; this code needs more work.

eyalroz · 2022-05-03T10:48:03Z

include/cxxopts.hpp

+ParseResult
+Options::parse(const std::string& argv_str)
+{
+  std::vector<std::string> tokens;


Why create all of these copies?

eyalroz · 2022-05-03T10:49:07Z

README.md

@@ -75,6 +75,16 @@ To parse the command line do:
 auto result = options.parse(argc, argv);
 ```

+Or if you have all the arguments of `argv` in a `std::string`, separated by


I'm not sure I'm happy with you forcing the use of std::string. What if I have a char*? I can't wrap it in a string, so you're forcing me to string-copy.

I think this is a legit concern, though it goes beyond the scope of the problem this patch is trying to target at. Also unlike the single string case, I cannot think of a use case where you are given all the arguments in a single char*. I don't see a trivial fix for this to avoid the extra copy of the whole string, but I am happy to learn more on it and address it when possible.

@huangzonghao : If you use a class/struct which can be implicitly constructed both from an std::string and from a char* - then you won't need the copy. That class is string_view... see my comment below.

eyalroz · 2022-05-03T10:51:59Z

include/cxxopts.hpp

+    tokens.push_back(token);
+  }
+
+  std::unique_ptr<const char*[]> argv(new const char*[tokens.size()]);


So, you're going through a bunch of hoops to create strings, which are copies of the original argv string, which itself may be a copy of something else - and now you're creating copies again?

Look, none of this copying needs to happen. Use either a string_view, or your own struct with a pointer and a length, or a pair of pointers.

string_view breaks the requirement of this project, and could potentially break the other projects that depend on this one. But you are right, we should be able to generate argv pointers based on the original string copy. I'll look into this later.

@huangzonghao : You're right, of course. By "or" I meant "or, actually, you could use"...

Also, one could use string_view lite, which is C++11-compatible. That's essentially the same thing but with more code... I've been known to implement "poor man's string_view" or "poor man's span" in some of my C++11 projects.

I would prefer not to bring in other libraries, and at the moment I still want to keep it C++11 compatible. However I think there is probably a nicer way to do this. I think you could probably just copy the whole string into a new buffer, replace all spaces with a zero, and store the pointers as you go in an argv vector.

eyalroz · 2022-05-03T10:53:38Z

test/options.cpp

@@ -21,6 +21,8 @@ class Argv {
      m_args.push_back(std::move(ptr));
      m_argv.get()[i] = m_args.back().get();

+      m_argv_str += std::string(m_argv.get()[i]) + std::string(" ");


Please don't burden the non-single-string code with this string copying.

Nearly all tests made for the (argc, argv) pair could be used for this single string code, unless we later on decide to design a separate set of tests for single string code and put them into a different cpp file. I would suggest we put the tests for (argc, argv) pair and single string together, so that whenever a new test is added, both functions can be tested.

I agree that this is not necessary. You really only need to test that the string splitting works.

Oh, sorry, I take it back! I didn't notice this is was in one of the tests... :-(

jarro2783 · 2022-05-03T23:19:12Z

test/options.cpp

  CHECK_THROWS_AS(result["nothing"].as<std::string>(), cxxopts::option_has_no_value_exception&);

+  auto str_result = options.parse(argv.argv_str());


I don't think duplicating all of the test cases is a good idea. If you really wanted to test both you might be able to use a catch SECTION to do it without duplication. But then I would be ok with factoring out the string splitting into its own class and just testing that splitting the strings up gives the correct results.

huangzonghao added 2 commits April 29, 2022 00:45

Update example code and readme for string parsing

4f7ab89

jarro2783 requested changes May 2, 2022

View reviewed changes

Use constref for string, smart pointer for char array, and address co…

fb50260

…mpilation warnings of 95c7709

huangzonghao force-pushed the master branch from b29390e to fb50260 Compare May 3, 2022 03:06

This comment was marked as resolved.

Sign in to view

huangzonghao added 2 commits May 3, 2022 04:17

Add test cases for string parsing

6c6deab

Revert changes made to example.cpp in 4f7ab89

ca98684

eyalroz suggested changes May 3, 2022

View reviewed changes

jarro2783 reviewed May 3, 2022

View reviewed changes

jarro2783 closed this Jul 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse command line arguments as a std::string #330

Parse command line arguments as a std::string #330

huangzonghao commented May 1, 2022 •

edited by jarro2783

Loading

eyalroz commented May 1, 2022

jarro2783 May 2, 2022

jarro2783 May 2, 2022

jarro2783 May 2, 2022

This comment was marked as resolved.

eyalroz left a comment

eyalroz May 3, 2022

eyalroz May 3, 2022

huangzonghao May 3, 2022

eyalroz May 3, 2022

eyalroz May 3, 2022

huangzonghao May 3, 2022

eyalroz May 3, 2022

jarro2783 May 3, 2022

eyalroz May 3, 2022

huangzonghao May 3, 2022

jarro2783 May 3, 2022

eyalroz May 3, 2022

jarro2783 May 3, 2022

		CHECK_THROWS_AS(result["nothing"].as<std::string>(), cxxopts::option_has_no_value_exception&);

		auto str_result = options.parse(argv.argv_str());

Parse command line arguments as a std::string #330

Parse command line arguments as a std::string #330

Conversation

huangzonghao commented May 1, 2022 • edited by jarro2783 Loading

eyalroz commented May 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

eyalroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huangzonghao commented May 1, 2022 •

edited by jarro2783

Loading